Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bitswap/client: add basic traceable blocks #308

Merged
merged 4 commits into from
Jul 26, 2023

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented May 18, 2023

Fixes #209

@Jorropo Jorropo requested a review from a team as a code owner May 18, 2023 18:26
@Jorropo
Copy link
Contributor Author

Jorropo commented May 18, 2023

This needs tests !

@BigLep
Copy link
Contributor

BigLep commented May 18, 2023

Also a changelog update?

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Merging #308 (e886cf5) into main (1f5df74) will decrease coverage by 0.52%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #308      +/-   ##
==========================================
- Coverage   49.71%   49.19%   -0.52%     
==========================================
  Files         248      245       -3     
  Lines       29933    29870      -63     
==========================================
- Hits        14882    14696     -186     
- Misses      13619    13746     +127     
+ Partials     1432     1428       -4     
Files Changed Coverage Δ
...wap/client/internal/notifications/notifications.go 73.25% <87.50%> (+1.10%) ⬆️
bitswap/client/client.go 84.64% <100.00%> (+2.43%) ⬆️

... and 13 files with indirect coverage changes

@Jorropo Jorropo force-pushed the add-bitswap-basic-introspectability branch from 4537dcf to a6bfd5e Compare May 18, 2023 18:43
@hacdias
Copy link
Member

hacdias commented May 24, 2023

This needs tests !

Any ETA to add the tests?

@hannahhoward
Copy link
Contributor

@Jorropo @hacdias I've rebased and added testing of the functionality. Note the tests are not actually running on CI cause of #327 . I have run them locally however and verified they pass.

@hannahhoward
Copy link
Contributor

also added Changelog, though I don't know if you all have a preferred format.

@davidd8
Copy link

davidd8 commented Jul 11, 2023

@Jorropo any updates on getting this PR merged? Based on @BigLep's comment in #209, I assume this will happen this week by 2023-07-14, but correct me if I'm wrong.

@hannahhoward
Copy link
Contributor

Following up: @BigLep this still isn't merged. It's not clear what's blocking -- can we move this along?

@Jorropo
Copy link
Contributor Author

Jorropo commented Jul 19, 2023

@hannahhoward my time is blocking, this is the next thing on the list https://github.com/orgs/ipfs/projects/16/views/17 and will get in for the next boxo release.

BTW This PR is not needed for the requirement that we passed in the slack thread, this was under nice to have.
The required per peer statistics are doable with the Tracer API. I'll implement an example of how you do that with peer-buckets in Kubo with prometheus.

@BigLep
Copy link
Contributor

BigLep commented Jul 25, 2023

I know there has been chatter about this in Slack. This PR will get landed this week as we'll include it in Kubo 0.22, which we're pushing to get an RC out ASAP.

@BigLep BigLep mentioned this pull request Jul 25, 2023
@hacdias hacdias force-pushed the add-bitswap-basic-introspectability branch from 5acf562 to e886cf5 Compare July 26, 2023 09:16
@hacdias hacdias merged commit 5ba947b into main Jul 26, 2023
@hacdias hacdias deleted the add-bitswap-basic-introspectability branch July 26, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Bitswap: better diagnostics and observability on client
5 participants